Fix: allow contained copies within child allocations - #1538
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughChild-pointer provenance now tracks allocation sizes for mallocs and CommDomain buffers. Orchestrator and L2 ChangesChild pointer provenance
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
94adb70 to
af47001
Compare
high-cloud
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/ut/py/test_worker/test_child_addr_guard.py (2)
238-247: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winInclude a negative-size case in the range rejection matrix.
The provenance contract in
python/simpler/worker.py(Lines 5668-5692) rejects negative sizes before native copying, but this parametrization covers only positive overflow and an out-of-range zero-byte destination. Add(0x2000, -1)and retain the no-native-call assertion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/ut/py/test_worker/test_child_addr_guard.py` around lines 238 - 247, Extend the parameterization in test_copy_to_rejects_range_past_allocation to include (0x2000, -1), and retain or add an assertion that fake.malloc/native copying is not invoked when the negative size is rejected.
264-265: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that rejected bounded copies never reach the native backend.
Both tests perform a successful copy first, so failure alone does not prove that the subsequent invalid request was blocked before native dispatch.
tests/ut/py/test_worker/test_child_addr_guard.py#L264-L265: assertfake.copy_to.call_count == 1after the rejected domain-window request.tests/ut/py/test_worker/test_child_addr_guard.py#L459-L460: assertchip.copy_to.call_count == 1after the rejected L2 request.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/ut/py/test_worker/test_child_addr_guard.py` around lines 264 - 265, Add native-dispatch assertions to both rejected bounded-copy tests: in tests/ut/py/test_worker/test_child_addr_guard.py lines 264-265, assert fake.copy_to.call_count == 1 after the invalid domain-window request; at lines 459-460, assert chip.copy_to.call_count == 1 after the invalid L2 request. Keep the existing successful-copy setup and ValueError assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/simpler/worker.py`:
- Line 463: Sort the entries in the __slots__ tuple alphabetically to satisfy
Ruff RUF023, changing the order of malloc_nbytes and domain_allocation_nbytes
while preserving both slot names.
---
Nitpick comments:
In `@tests/ut/py/test_worker/test_child_addr_guard.py`:
- Around line 238-247: Extend the parameterization in
test_copy_to_rejects_range_past_allocation to include (0x2000, -1), and retain
or add an assertion that fake.malloc/native copying is not invoked when the
negative size is rejected.
- Around line 264-265: Add native-dispatch assertions to both rejected
bounded-copy tests: in tests/ut/py/test_worker/test_child_addr_guard.py lines
264-265, assert fake.copy_to.call_count == 1 after the invalid domain-window
request; at lines 459-460, assert chip.copy_to.call_count == 1 after the invalid
L2 request. Keep the existing successful-copy setup and ValueError assertions
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 87bd3dc4-1b68-45ba-8341-cbfb294840a5
📒 Files selected for processing (3)
python/simpler/orchestrator.pypython/simpler/worker.pytests/ut/py/test_worker/test_child_addr_guard.py
Track malloc and CommDomain extents in child-pointer provenance. Existing copy_to calls may target a contained interior range while wrong-worker, stale, and out-of-bounds ranges are rejected before the native copy.
af47001 to
09d1788
Compare
1 participant
Summary
Orchestrator.copy_to(worker_id, dst, src, size)APIfree()and child-memory task dispatchWhy
copy_to()previously validated only an exact registered pointer. A valid chunked copy into a live CommDomain window therefore failed as soon as the destination advanced fromwindow_baseto an interior address.The validation now treats the copy destination as a byte range and proves that the complete range belongs to a live allocation on the selected worker. Exact destinations retain an O(1) lookup; only interior destinations require scanning that worker's live provenance entries. No new public API or caller migration is needed.
Validation
python -m pytest -q tests/ut/py/test_worker/test_child_addr_guard.py— 44 passedpython -m pytest -q tests/ut— 847 passed, 2 skippedpython -m pre_commit run --files python/simpler/orchestrator.py python/simpler/worker.py tests/ut/py/test_worker/test_child_addr_guard.py— passedgit diff --check— passed